interface/src/components/ChannelSettingCard.tsx (1)

642-652: Add lightweight client-side URL validation for Mattermost server URL.

Line 643 currently only checks non-empty input. Validating URL format here gives faster, clearer feedback before submit.

Suggested patch

 		} else if (platform === "mattermost") {
-			if (!credentialInputs.mattermost_base_url?.trim()) {
+			const baseUrl = credentialInputs.mattermost_base_url?.trim();
+			if (!baseUrl) {
 				setMessage({text: "Server URL is required", type: "error"});
 				return;
 			}
+			try {
+				new URL(baseUrl);
+			} catch {
+				setMessage({text: "Server URL must be a valid absolute URL", type: "error"});
+				return;
+			}
 			if (!credentialInputs.mattermost_token?.trim()) {
 				setMessage({text: "Access token is required", type: "error"});
 				return;
 			}
-			credentials.mattermost_base_url = credentialInputs.mattermost_base_url.trim();
+			credentials.mattermost_base_url = baseUrl;
 			credentials.mattermost_token = credentialInputs.mattermost_token.trim();
 		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@interface/src/components/ChannelSettingCard.tsx` around lines 642 - 652, In
the platform === "mattermost" branch of ChannelSettingCard, replace the simple
non-empty check for credentialInputs.mattermost_base_url with a lightweight
client-side URL validation: trim the input, attempt to validate that it is a
well-formed HTTP/HTTPS URL (for example with new URL(trimmed) and checking
protocol is http: or https:), and if invalid call setMessage({text: "Invalid
server URL", type: "error"}) and return; only after it passes validation assign
credentials.mattermost_base_url = trimmed and proceed; keep the existing token
trim/validation logic intact.

src/messaging/mattermost.rs (8)

518-523: ⚠️ Potential issue | 🟠 Major

StreamStart misses thread fallback metadata.

This path only uses mattermost_root_id; it should also fallback to REPLY_TO_MESSAGE_ID like the text path does.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 518 - 523,
OutboundResponse::StreamStart currently extracts only
message.metadata["mattermost_root_id"] and calls start_typing without the same
thread fallback used elsewhere; update the StreamStart branch to follow the
text-path logic by checking message.metadata for "mattermost_root_id" and, if
absent, falling back to REPLY_TO_MESSAGE_ID before using that value for threaded
behavior (the same metadata lookup pattern used in the text response branch),
ensuring start_typing/channel handling uses the resolved thread id from
message.metadata.

---

154-186: ⚠️ Potential issue | 🟠 Major

Typing tasks are not safely replaced/terminated.

Line 185 overwrites existing channel task handles without explicit abort, and Line 770 clears handles on shutdown without aborting them.

#!/bin/bash
# Verify typing task lifecycle handling in start_typing/shutdown
rg -n -C4 'async fn start_typing|typing_tasks.*insert|async fn shutdown|typing_tasks.*clear' src/messaging/mattermost.rs

💡 Proposed fix

     async fn start_typing(&self, channel_id: &str) {
+        self.stop_typing(channel_id).await;
         let Some(user_id) = self.bot_user_id.get().cloned() else {
             return;
         };
@@
-        self.typing_tasks.write().await.clear();
+        let mut typing_tasks = self.typing_tasks.write().await;
+        for (_, handle) in typing_tasks.drain() {
+            handle.abort();
+        }

Also applies to: 770-771

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 154 - 186, The start_typing
function currently inserts a new tokio::task::JoinHandle into typing_tasks
without stopping any existing task for the same channel; update start_typing to
check typing_tasks (by channel_id) for an existing handle, call abort() on it
and optionally await its completion or ignore the result before inserting the
new handle so you don't leak duplicate tasks; likewise modify the shutdown logic
(the code that currently clears typing_tasks) to instead iterate all stored
JoinHandles, call abort() on each handle (and await or detach as appropriate)
before removing them so all background typing tasks are explicitly terminated —
refer to the start_typing method, typing_tasks field and the shutdown function
to locate the changes.

---

659-669: ⚠️ Potential issue | 🟠 Major

Post creation after file upload lacks HTTP status handling.

Both paths send POST /posts and ignore non-success responses, which can silently drop file messages.

#!/bin/bash
# Verify /posts calls after file upload without explicit status checks
rg -n -C6 'failed to create post with file|post\(self\.api_url\("/posts"\)\)' src/messaging/mattermost.rs

Also applies to: 818-829

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 659 - 669, The POST to /posts after
file upload (the request built with
self.client.post(self.api_url("/posts")).bearer_auth(self.token.as_ref()).json(...).send().await.context("failed
to create post with file")?) does not check HTTP status and can silently ignore
non-2xx responses; update the call to inspect the Response returned by send()
(e.g., assign to a variable), call response.error_for_status() or check
status().is_success(), and return a contextual error (including status and body
text) if not successful. Apply the same fix for the other /posts call that uses
the "failed to create post" context (the second occurrence around the other
upload path) so both paths surface non-success HTTP responses.

---

762-764: ⚠️ Potential issue | 🟡 Minor

Don’t silently discard shutdown send errors.

Line 763 uses let _ = on a Result; handle/log it (or use .ok() if intentionally drop-safe for channel send).

As per coding guidelines: "Don't silently discard errors. No let _ = on Results. Handle them, log them, or propagate them. The only exception is .ok() on channel sends where the receiver may be dropped."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 762 - 764, The code currently
discards the Result from sending the shutdown signal with "let _ =
tx.send(()).await" on the shutdown_tx channel; change this to explicitly handle
the Result from tx.send(()).await (e.g., use if let Err(err) = tx.send(()).await
{ log the error with tracing::warn! or tracing::error! } or, if it's
intentionally safe to ignore a dropped receiver, call .ok() on the Result) so
you no longer silently discard errors; locate the send call on the shutdown_tx
(the shutdown_tx.write().await.take() -> tx.send(()).await expression) and
replace the "let _ =" pattern with explicit logging or .ok() as appropriate.

---

583-587: ⚠️ Potential issue | 🟡 Minor

Fail fast when bot_user_id is uninitialized for reactions.

Defaulting to an empty user ID produces opaque server-side reaction errors.

💡 Proposed fix

-                let bot_user_id = self
-                    .bot_user_id
-                    .get()
-                    .map(|s| s.as_ref().to_string())
-                    .unwrap_or_default();
+                let bot_user_id = self
+                    .bot_user_id
+                    .get()
+                    .ok_or_else(|| anyhow::anyhow!("bot_user_id not initialized"))?
+                    .as_ref()
+                    .to_string();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 583 - 587, The code currently
defaults bot_user_id to an empty string which causes opaque server errors when
sending reactions; replace the unwrap_or_default usage on
self.bot_user_id.get().map(|s| s.as_ref().to_string()).unwrap_or_default() with
an explicit failure path: either call expect/panic with a clear message like
"bot_user_id not initialized for reactions" or, preferably, change the
surrounding handler to return a Result and use ok_or_else / map_err to return a
descriptive error (e.g., "missing bot_user_id") so callers can handle it instead
of sending an empty ID; update any call sites to handle the Result if you choose
propagation.

---

308-317: ⚠️ Potential issue | 🟠 Major

Check /users/me HTTP status before JSON parsing.

On auth/config failures, this currently reports a parse error instead of surfacing status/body details.

#!/bin/bash
# Verify /users/me path currently parses JSON directly
rg -n -C8 'get\(self\.api_url\("/users/me"\)\)|failed to parse user response' src/messaging/mattermost.rs

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 308 - 317, The current call to
self.client.get(self.api_url("/users/me")).bearer_auth(self.token.as_ref()).send().await...
then .json().await can mask non-2xx responses as JSON parse errors; modify the
flow in the /users/me request (the block that assigns to let me: MattermostUser)
to inspect the HTTP response status after send() and before json(): await the
Response from send(), check response.status().is_success() (or use
response.error_for_status()/error_for_status_ref()), and if not successful read
the response text (response.text().await) and return an error/context that
includes the status code and body; only call response.json().await when the
status is successful so parse errors truly reflect JSON problems. Ensure you
reference the same calls (self.client.get, api_url("/users/me"), bearer_auth,
send, json) when making the change.

---

560-567: ⚠️ Potential issue | 🟠 Major

Final stream edit can exceed Mattermost max post length.

Line 564 edits with full accumulated_text; large streams can fail finalization and leave placeholder/truncated content.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 560 - 567, The final stream edit in
the OutboundResponse::StreamEnd branch uses active.accumulated_text directly and
can exceed Mattermost's max post length; before calling
self.edit_post(&active.post_id, &active.accumulated_text).await truncate or
otherwise constrain active.accumulated_text to the Mattermost max (use a
MAX_POST_LENGTH constant or a helper like truncate_to_max_post_length) and log a
warning if truncation occurs, so edit_post receives a length-safe string and
finalization won't fail.

---

1142-1149: ⚠️ Potential issue | 🔴 Critical

Unsafe UTF-8 slicing can panic on non-ASCII input.

Line 1142 slices with [..max_len] before finding a char boundary.

#!/bin/bash
# Verify split_message uses direct byte slicing before boundary correction
rg -n -C4 'let search_region = &remaining\[\.\.max_len\];|floor_char_boundary' src/messaging/mattermost.rs

💡 Proposed fix

-        let search_region = &remaining[..max_len];
+        let search_end = remaining.floor_char_boundary(max_len);
+        let search_region = &remaining[..search_end];
         let break_point = search_region
             .rfind('\n')
             .or_else(|| search_region.rfind(' '))
-            .unwrap_or(max_len);
+            .unwrap_or(search_end);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 1142 - 1149, The code currently
does unsafe byte slicing with let search_region = &remaining[..max_len]; which
can panic on UTF-8 boundaries; change it to compute a safe byte index for the
desired maximum by using
remaining.char_indices().nth(max_len).map(|(i,_)|i).unwrap_or(remaining.len())
(or similar) and then set search_region = &remaining[..safe_index]; keep using
rfind to find break_point and pass that byte index into floor_char_boundary
before slicing and pushing into chunks (update the logic around search_region,
break_point, floor_char_boundary, and chunks.push to use the computed safe byte
index rather than direct ..max_len slicing).

src/messaging/mattermost.rs (1)

538-553: Avoid holding active_messages write lock across awaited network I/O.

Line 552 awaits edit_post while the write lock is held, which can stall other streams and status updates.

💡 Proposed refactor

             OutboundResponse::StreamChunk(chunk) => {
-                let mut active_messages = self.active_messages.write().await;
-                if let Some(active) = active_messages.get_mut(&message.id) {
+                let mut update: Option<(Arc<str>, String)> = None;
+                {
+                    let mut active_messages = self.active_messages.write().await;
+                    if let Some(active) = active_messages.get_mut(&message.id) {
                         active.accumulated_text.push_str(&chunk);
@@
-                        if let Err(error) = self.edit_post(&active.post_id, &display_text).await {
-                            tracing::warn!(%error, "failed to edit streaming message");
-                        }
+                        update = Some((active.post_id.clone(), display_text));
                         active.last_edit = Instant::now();
                     }
                 }
+                if let Some((post_id, display_text)) = update
+                    && let Err(error) = self.edit_post(&post_id, &display_text).await
+                {
+                    tracing::warn!(%error, "failed to edit streaming message");
+                }
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 538 - 553, The code currently holds
the active_messages write lock while awaiting self.edit_post, which can block
other tasks; fix by extracting/cloning the needed data (e.g., clone
active.post_id and active.accumulated_text or compute display_text) while
holding the lock, then drop the lock before calling self.edit_post(&post_id,
&display_text). After the await, reacquire the write lock to update
active.last_edit (and any other state) only if the edit succeeded; use the same
symbols active_messages, active.accumulated_text, active.post_id, edit_post,
STREAM_EDIT_THROTTLE, and MAX_MESSAGE_LENGTH to locate and implement the change.

src/messaging/mattermost.rs (2)

840-842: ⚠️ Potential issue | 🟡 Minor

Don’t silently discard shutdown signal errors.

let _ = tx.send(()).await hides failure to notify the websocket task.

As per coding guidelines: "Don't silently discard errors. No let _ = on Results. Handle them, log them, or propagate them. The only exception is .ok() on channel sends where the receiver may be dropped"

Suggested fix

         if let Some(tx) = self.shutdown_tx.write().await.take() {
-            let _ = tx.send(()).await;
+            if let Err(error) = tx.send(()).await {
+                tracing::debug!(%error, adapter = %self.runtime_key, "shutdown receiver already closed");
+            }
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 840 - 842, The shutdown send
currently discards errors with `let _ = tx.send(()).await;`; update the code
around `self.shutdown_tx.write().await.take()` so the Result from
`tx.send(()).await` is not silently ignored—handle the error by at minimum
logging it (e.g., via the module logger or `error!`) or propagating it upward;
ensure the failure to notify the websocket task is visible (include context like
"failed to send shutdown signal to websocket task") and keep the existing
`take()` behavior on `shutdown_tx`.

---

916-927: ⚠️ Potential issue | 🟠 Major

broadcast() file uploads don’t validate /posts HTTP status.

This path can silently report success even when Mattermost rejects the post (401/403/413/etc).

Suggested fix

-                self.client
+                let post_response = self.client
                     .post(self.api_url("/posts"))
                     .bearer_auth(self.token.as_ref())
                     .json(&serde_json::json!({
                         "channel_id": target,
                         "message": caption.unwrap_or_default(),
                         "file_ids": file_ids,
                     }))
                     .send()
                     .await
                     .context("failed to create post with file")?;
+                let post_status = post_response.status();
+                if !post_status.is_success() {
+                    let body = post_response.text().await.unwrap_or_default();
+                    return Err(anyhow::anyhow!(
+                        "mattermost POST /posts (broadcast file) failed with status {}: {body}",
+                        post_status.as_u16()
+                    ).into());
+                }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 916 - 927, The POST to /posts in
broadcast() currently awaits send() but ignores the HTTP status; change it to
capture the Response from
self.client.post(self.api_url("/posts")).bearer_auth(...).json(...).send().await,
then validate the status (e.g., use resp.error_for_status() or check
resp.status().is_success()), and on non-success return a contextual error that
includes the HTTP status and response body/text to surface Mattermost errors
(401/403/413/etc.); update the error message passed to .context("failed to
create post with file") to include these details.

src/api/messaging.rs (2)

518-570: Rename mm to a descriptive variable name.

Please avoid abbreviated local names here (e.g., mattermost_config) for readability and consistency with repo style.

As per coding guidelines **/*.rs: "Don't abbreviate variable names. Use queue not q, message not msg, channel not ch. Common abbreviations like config are fine".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/api/messaging.rs` around lines 518 - 570, The local variable `mm` in the
Mattermost block is an unclear abbreviation; rename it to a descriptive
identifier like `mattermost_config` (or similar) wherever it's used in this
block (the binding from doc.get("messaging").and_then(|m| m.get("mattermost")),
the checks for base_url/token/enabled, the `named_instances` extraction, and the
loop that computes `instance_name`, `instance_enabled`, and
`instance_configured`) so the code remains readable and consistent; update all
references passed into functions such as push_instance_status and calls to
normalize_adapter_selector and ensure no other symbol names change.

---

1331-1347: Deduplicate Mattermost base URL validation.

The same validation block appears in both default and named instance paths. Extracting a helper will reduce drift risk.

♻️ Proposed refactor

+fn is_valid_mattermost_origin(base_url: &str) -> bool {
+    url::Url::parse(base_url)
+        .map(|parsed_url| {
+            parsed_url.path() == "/"
+                && parsed_url.query().is_none()
+                && parsed_url.fragment().is_none()
+        })
+        .unwrap_or(false)
+}
...
-                    if let Some(url) = &credentials.mattermost_base_url {
-                        if url::Url::parse(url)
-                            .map(|u| u.path() != "/" || u.query().is_some() || u.fragment().is_some())
-                            .unwrap_or(true)
-                        {
+                    if let Some(url) = &credentials.mattermost_base_url {
+                        if !is_valid_mattermost_origin(url) {
                             return Ok(Json(MessagingInstanceActionResponse {
                                 success: false,
                                 message: format!("invalid mattermost base_url: must be an origin URL (e.g. https://mm.example.com)"),
                             }));
                         }
                         platform_table["base_url"] = toml_edit::value(url.as_str());
                     }
...
-                    if let Some(url) = &credentials.mattermost_base_url {
-                        if url::Url::parse(url)
-                            .map(|u| u.path() != "/" || u.query().is_some() || u.fragment().is_some())
-                            .unwrap_or(true)
-                        {
+                    if let Some(url) = &credentials.mattermost_base_url {
+                        if !is_valid_mattermost_origin(url) {
                             return Ok(Json(MessagingInstanceActionResponse {
                                 success: false,
                                 message: format!("invalid mattermost base_url: must be an origin URL (e.g. https://mm.example.com)"),
                             }));
                         }
                         instance_table["base_url"] = toml_edit::value(url.as_str());
                     }

Also applies to: 1457-1473

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/api/messaging.rs` around lines 1331 - 1347, Duplicate Mattermost base_url
parsing/validation occurs in the "mattermost" branch (checking
credentials.mattermost_base_url with url::Url::parse and the path/query/fragment
conditions) and should be extracted into a small helper to avoid drift; add a
function like validate_mattermost_base_url(url: &str) -> Result<(), String> (or
similar) that encapsulates the parse and the condition (path == "/" &&
query.is_none() && fragment.is_none()), call that helper from both places, and
on Err return the same Json(MessagingInstanceActionResponse { success: false,
message: ... }) behavior; update places that currently set
platform_table["base_url"] to use the validated url after the helper returns Ok.

src/messaging/mattermost.rs (2)

432-433: ⚠️ Potential issue | 🟡 Minor

Do not discard lifecycle Results silently.

OnceCell::set(...).ok() (Lines 432-433) and let _ = tx.send(()).await (Line 916) drop error context and make startup/shutdown state transitions harder to diagnose.

🛠️ Proposed explicit handling

-        self.bot_user_id.set(user_id.clone()).ok();
-        self.bot_username.set(username.clone()).ok();
+        if self.bot_user_id.set(user_id.clone()).is_err() {
+            tracing::debug!(adapter = %self.runtime_key, "bot_user_id already initialized");
+        }
+        if self.bot_username.set(username.clone()).is_err() {
+            tracing::debug!(adapter = %self.runtime_key, "bot_username already initialized");
+        }
@@
-        if let Some(tx) = self.shutdown_tx.write().await.take() {
-            let _ = tx.send(()).await;
-        }
+        if let Some(tx) = self.shutdown_tx.write().await.take()
+            && let Err(error) = tx.send(()).await
+        {
+            tracing::debug!(%error, adapter = %self.runtime_key, "shutdown signal receiver dropped");
+        }

As per coding guidelines "Don't silently discard errors. No `let _ =` on Results. Handle them, log them, or propagate them. The only exception is `.ok()` on channel sends where the receiver may be dropped".

Also applies to: 915-917

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 432 - 433, The two OnceCell set
calls (self.bot_user_id.set(...) and self.bot_username.set(...)) and the
tx.send(()).await are currently discarding Results via .ok() and let _ = ..., so
replace those silent drops with explicit handling: check the Result from
self.bot_user_id.set(...) and self.bot_username.set(...) (or use try_set/expect
if startup must fail) and log or return an Err with contextual messages if
setting fails, and handle tx.send(()).await by matching the Result and logging
the send error or propagating it instead of ignoring it; update places
referencing bot_user_id, bot_username, and the tx.send call to use match/if let
Err(e) { /* log e or return Err(e) */ } so lifecycle failures are surfaced.

---

991-1002: ⚠️ Potential issue | 🟠 Major

Check /posts status in the broadcast file path.

At Line 991, non-2xx responses from creating the post after upload are currently treated as success, so file broadcasts can fail silently.

🧪 Proposed status/body handling

-                self.client
+                let post_response = self.client
                     .post(self.api_url("/posts"))
                     .bearer_auth(self.token.as_ref())
                     .json(&serde_json::json!({
                         "channel_id": target,
                         "message": caption.unwrap_or_default(),
                         "file_ids": file_ids,
                     }))
                     .send()
                     .await
                     .context("failed to create post with file")?;
+                let post_status = post_response.status();
+                if !post_status.is_success() {
+                    let body = post_response.text().await.unwrap_or_default();
+                    return Err(anyhow::anyhow!(
+                        "mattermost POST /posts (broadcast file) failed with status {}: {body}",
+                        post_status.as_u16()
+                    ).into());
+                }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 991 - 1002, The post-create call
currently ignores non-2xx responses and treats them as success; change the
self.client.post(self.api_url("/posts")).bearer_auth(self.token.as_ref()).json(...)...send().await
call to capture the Response into a variable, await and then verify
response.status().is_success(); if not successful, read response.text().await
and return an error (or context) that includes the HTTP status and response body
so failures aren't silent. Replace the existing chained
.send().await.context("failed to create post with file")? with this explicit
check around the response from the post request.

src/config/types.rs (2)

1565-1576: Consolidate Mattermost channel filtering to one code path.

channel_ids for Mattermost is checked in both the generic block and a Mattermost-specific block, which duplicates logic and risks drift.

♻️ Proposed simplification

-        // Mattermost channel filter
-        if !self.channel_ids.is_empty() && self.channel == "mattermost" {
-            let message_channel = message
-                .metadata
-                .get("mattermost_channel_id")
-                .and_then(|v| v.as_str());
-            if !self.channel_ids.iter().any(|id| Some(id.as_str()) == message_channel) {
-                return false;
-            }
-        }

Also applies to: 1610-1619

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/config/types.rs` around lines 1565 - 1576, The Mattermost channel ID
check is duplicated across the generic channel matching and a
Mattermost-specific block; consolidate by keeping a single check path: compute
mattermost_channel once from message.metadata (as currently done) and include it
only in the unified direct_match expression that checks message_channel,
slack_channel, twitch_channel and mattermost_channel against self.channel_ids
(using consistent to_string() conversion where needed), then remove the
redundant Mattermost-specific containment check in the other block (the similar
logic around the 1610-1619 region) so all channel filtering uses the same
variables and comparison logic.

---

1889-1891: Use descriptive closure variable names.

Please rename i to instance for readability and guideline alignment.

✏️ Small cleanup

-            mattermost.instances.iter().map(|i| i.name.as_str()),
+            mattermost
+                .instances
+                .iter()
+                .map(|instance| instance.name.as_str()),

As per coding guidelines: "Don't abbreviate variable names. Use queue not q, message not msg, channel not ch."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/config/types.rs` around lines 1889 - 1891, The closure passed to
validate_instance_names uses the abbreviated variable name `i`; rename it to a
descriptive name like `instance` (e.g., change
`mattermost.instances.iter().map(|i| i.name.as_str())` to use `|instance|
instance.name.as_str()`) to improve readability and follow naming guidelines;
ensure any other occurrences in the same expression or nearby bindings are
updated to the new name.

src/messaging/mattermost.rs (1)

995-1005: ⚠️ Potential issue | 🟠 Major

Missing HTTP status check for post creation after file upload.

The /posts response status is not checked, so 4xx/5xx errors are silently ignored. This is inconsistent with the same logic in respond() (lines 819-826) which properly checks post_status.is_success().

🐛 Proposed fix

-                self.client
+                let post_response = self
+                    .client
                     .post(self.api_url("/posts"))
                     .bearer_auth(self.token.as_ref())
                     .json(&serde_json::json!({
                         "channel_id": target,
                         "message": caption.unwrap_or_default(),
                         "file_ids": file_ids,
                     }))
                     .send()
                     .await
                     .context("failed to create post with file")?;
+                let post_status = post_response.status();
+                if !post_status.is_success() {
+                    let body = post_response.text().await.unwrap_or_default();
+                    return Err(anyhow::anyhow!(
+                        "mattermost POST /posts (file broadcast) failed with status {}: {body}",
+                        post_status.as_u16()
+                    ).into());
+                }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 995 - 1005, The post-creation call
using self.client.post(self.api_url("/posts")) does not check the HTTP response
status so 4xx/5xx failures are ignored; change the call in the block that posts
with file_ids to capture the response (await the send into a variable), check
response.status().is_success(), and return an error/context if not successful
(similar to the existing logic in respond()), including the status and any body
text in the error message to aid debugging; locate the code around the POST to
"/posts" where .bearer_auth, .json and .send().await are used and apply the same
status-check pattern as in respond().

src/messaging/mattermost.rs (2)

490-490: Consider logging malformed WebSocket event envelopes.

JSON parse failures for the outer event structure are silently dropped, which can make debugging integration issues harder. The post payload parsing (lines 499-506) already logs errors correctly.

🔧 Suggested fix

-                                            if let Ok(event) = serde_json::from_str::<MattermostWsEvent>(&text) {
+                                            let event = match serde_json::from_str::<MattermostWsEvent>(&text) {
+                                                Ok(e) => e,
+                                                Err(error) => {
+                                                    tracing::trace!(%error, "failed to parse Mattermost WS event envelope");
+                                                    continue;
+                                                }
+                                            };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` at line 490, The WebSocket message parsing
silently ignores failures when deserializing into MattermostWsEvent; update the
code around the serde_json::from_str::<MattermostWsEvent>(&text) branch to log
parse errors (including the raw text and the serde error) rather than dropping
them silently so malformed outer envelopes are visible; locate the parsing site
that uses MattermostWsEvent and add a processLogger/error (or tracing::error)
call that records the error and the offending text before continuing.

---

1484-1510: Consider adding a test for channel_filter with unconfigured team.

The existing tests verify fail-closed when team_id is absent, but there's no test case for when channel_filter is configured and a message comes from a team that has no entry in the map. This would help catch the fail-open behavior flagged above.

🧪 Suggested test case

#[test]
fn channel_filter_rejects_team_without_entry() {
    let p = post("user1", "chan1", None);
    let mut cf = HashMap::new();
    cf.insert("team1".into(), vec!["chan1".into()]);
    let perms = MattermostPermissions { team_filter: None, channel_filter: cf, dm_allowed_users: vec![] };
    // team2 has no entry in channel_filter → should reject (fail-closed)
    assert!(bmfp(&p, "bot", Some("team2"), &perms).is_none());
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 1484 - 1510, Add a unit test that
verifies bmfp rejects messages from a team not present in
MattermostPermissions.channel_filter: create a post via
post("user1","chan1",None), build channel_filter HashMap containing only "team1"
(with "chan1"), construct MattermostPermissions with that channel_filter, then
call bmfp(&p, "bot", Some("team2"), &perms) and assert it returns None to ensure
fail-closed behavior when the team key is missing.

src/messaging/mattermost.rs (1)

432-433: ⚠️ Potential issue | 🟡 Minor

Avoid silently discarding startup/shutdown Results.

OnceCell::set(...).ok() (Line 432-433) and let _ = tx.send(()).await (Line 932) hide failure paths. Handle/log the former explicitly, and use the channel-send exception pattern (.ok()) for the latter.

🔧 Proposed fix

-        self.bot_user_id.set(user_id.clone()).ok();
-        self.bot_username.set(username.clone()).ok();
+        if self.bot_user_id.set(user_id.clone()).is_err() {
+            tracing::debug!(adapter = %self.runtime_key, "bot_user_id already initialized");
+        }
+        if self.bot_username.set(username.clone()).is_err() {
+            tracing::debug!(adapter = %self.runtime_key, "bot_username already initialized");
+        }
@@
         if let Some(tx) = self.shutdown_tx.write().await.take() {
-            let _ = tx.send(()).await;
+            tx.send(()).await.ok();
         }

#!/bin/bash
rg -n 'bot_user_id\.set\(.*\)\.ok\(\)|bot_username\.set\(.*\)\.ok\(\)|let _ = tx\.send\(\(\)\)\.await' src/messaging/mattermost.rs

Expected result after the fix: no matches.

As per coding guidelines: "Don't silently discard errors. No let _ = on Results. Handle them, log them, or propagate them. The only exception is .ok() on channel sends where the receiver may be dropped".

Also applies to: 931-933

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 432 - 433, Replace the silent
discards: do not call .ok() on OnceCell::set for self.bot_user_id and
self.bot_username; instead capture the Result returned by
self.bot_user_id.set(user_id.clone()) and
self.bot_username.set(username.clone()) and handle the Err case (e.g., log an
error or warning with the existing logger/tracing macro and include the
attempted value or reason). For the channel send use the accepted channel-send
pattern: call tx.send(()).await and explicitly ignore the Ok or call .ok() on
the awaited Result only (e.g., let _ = tx.send(()).await.ok();) so the
receiver-dropped case is intentionally silenced while other set failures are
logged/handled. Ensure you reference the exact symbols self.bot_user_id.set,
self.bot_username.set and tx.send(()).await when making the changes.

src/config/types.rs (1)

2706-2717: ⚠️ Potential issue | 🟠 Major

Named Mattermost instance secrets are not registered as system secrets.

MattermostConfig supports instances, but MATTERMOST_TOKEN is registered without an instance_pattern. That breaks parity with other adapters and leaves MATTERMOST_{INSTANCE}_TOKEN unmatched by registry-based categorization.

🔧 Proposed fix

 impl SystemSecrets for MattermostConfig {
@@
     fn secret_fields() -> &'static [SecretField] {
         &[
             SecretField {
                 toml_key: "token",
                 secret_name: "MATTERMOST_TOKEN",
-                instance_pattern: None,
+                instance_pattern: Some(InstancePattern {
+                    platform_prefix: "MATTERMOST",
+                    field_suffix: "TOKEN",
+                }),
             },
             SecretField {
                 toml_key: "base_url",
                 secret_name: "MATTERMOST_BASE_URL",
-                instance_pattern: None,
+                instance_pattern: Some(InstancePattern {
+                    platform_prefix: "MATTERMOST",
+                    field_suffix: "BASE_URL",
+                }),
             },
         ]
     }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/config/types.rs` around lines 2706 - 2717, The Mattermost secret_fields
list registers MATTERMOST_TOKEN and MATTERMOST_BASE_URL without
instance_pattern, so per-instance secrets like MATTERMOST_{INSTANCE}_TOKEN are
not recognized; update the SecretField entries inside the secret_fields()
function for MattermostConfig (the SecretField with toml_key "token" and the one
with "base_url") to include appropriate instance_pattern values (e.g. pattern
matching "{instance}_TOKEN" and "{instance}_BASE_URL" or the project’s existing
instance placeholder format) so named Mattermost instance secrets are registered
like the other adapters.

src/config/types.rs (1)

1543-1583: Deduplicate Mattermost channel_ids filtering in matches_route.

At Line 1576, Mattermost channel matching is already part of direct_match; the second Mattermost-only check at Line 1611 repeats the same constraint. Keeping one check reduces drift risk.

♻️ Simplify duplicated logic

-        // Mattermost channel filter
-        if !self.channel_ids.is_empty() && self.channel == "mattermost" {
-            let message_channel = message
-                .metadata
-                .get("mattermost_channel_id")
-                .and_then(|v| v.as_str());
-            if !self.channel_ids.iter().any(|id| Some(id.as_str()) == message_channel) {
-                return false;
-            }
-        }

Also applies to: 1611-1619

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/config/types.rs` around lines 1543 - 1583, The matches_route logic
duplicates the Mattermost channel check: remove the second Mattermost-only
filter block that separately checks mattermost_channel against self.channel_ids
since mattermost_channel is already included in direct_match (the variables
mattermost_channel, direct_match, parent_match, and self.channel_ids are in this
function); ensure you keep the parent_channel parent_match logic unchanged and
rely on direct_match to cover Slack/Twitch/Mattermost direct matches to avoid
redundant checks.

src/config/types.rs (1)

1880-1880: Use a descriptive closure variable name instead of i.

At Line 1880, map(|i| i.name.as_str()) should use a descriptive name for readability and guideline compliance.

✏️ Suggested tweak

-            mattermost.instances.iter().map(|i| i.name.as_str()),
+            mattermost.instances.iter().map(|instance| instance.name.as_str()),

As per coding guidelines: "Don't abbreviate variable names. Use queue not q, message not msg, channel not ch."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/config/types.rs` at line 1880, The closure in the iterator mapping uses a
non-descriptive variable name `i`; change the closure in
mattermost.instances.iter().map(|i| i.name.as_str()) to use a clear name like
`instance` (e.g., map(|instance| instance.name.as_str())) to improve readability
and follow naming guidelines while keeping behavior identical.

src/messaging/mattermost.rs (1)

1455-1461: Rename abbreviated test helpers for clarity and guideline compliance.

bmfp, bmfp_named, and single-letter params (p) are harder to scan in this already dense test module.

As per coding guidelines: "Don't abbreviate variable names. Use queue not q, message not msg, channel not ch. Common abbreviations like config are fine".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 1455 - 1461, Rename the abbreviated
test helpers and single-letter parameters for clarity: change functions bmfp and
bmfp_named to descriptive names (e.g., build_message_from_mattermost_post and
build_message_from_mattermost_post_named) and rename the parameter p to post
(and any other single-letter params) in their signatures and all local call
sites; update the calls to build_message_from_post inside these helpers to use
the new parameter names and ensure bot_id, bot_username, team_id, perms,
display_name, and channel_name arguments remain unchanged so behavior is
preserved.

src/messaging/mattermost.rs (1)

1177-1183: ⚠️ Potential issue | 🟠 Major

Set the directed-message flag for replies and DMs, not just @mentions.

passes_require_mention() uses mattermost_mentions_or_replies_to_bot as the adapter's allow signal. This only checks for an @{bot_username} substring, so thread replies to the bot—and DMs when a binding has require_mention = true—still get dropped.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 1177 - 1183,
passes_require_mention() relies on the metadata key
"mattermost_mentions_or_replies_to_bot", but the current mentions_bot
calculation only checks post.message.contains(format!("@{bot_username}"));
update the logic around mentions_bot (the let mentions_bot = ... and subsequent
metadata.insert) to also set true when the message is a direct message (check
post.channel_type or equivalent DM indicator) and when it is a thread reply to a
post authored by the bot (check post.root_id, fetch the parent post and compare
parent.user_id to bot_user_id); keep the original `@mention` test as well and then
insert the combined boolean into the same metadata key so replies and DMs are
treated as directed messages.

src/messaging/mattermost.rs (1)

132-144: Consider rejecting unsupported schemes instead of passing through.

The other => other fallback at line 137 silently passes through unknown schemes, which would produce a broken WebSocket URL. Given that the constructor validates the URL structure, this is unlikely to be reached, but failing explicitly would be clearer.

💡 Suggested defensive fix

         url.set_scheme(match self.base_url.scheme() {
             "https" => "wss",
             "http" => "ws",
-            other => other,
+            other => {
+                tracing::warn!(scheme = other, "unsupported base_url scheme for websocket");
+                other
+            }
         })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 132 - 144, The ws_url method
currently silently passes through unknown schemes via the match fallback (other
=> other) which can produce invalid websocket URLs; update ws_url to explicitly
reject unsupported schemes by replacing the fallback with an explicit failure
that includes the offending scheme (e.g., panic!/expect with a message like
"unsupported URL scheme: {scheme}"), so in the match over self.base_url.scheme()
only "http" => "ws" and "https" => "wss" are accepted and any other scheme
causes a clear, immediate error; make this change inside the ws_url function
referencing self.base_url and the scheme substitution to help locate the code.

src/api/bindings.rs (1)

737-737: Replace abbreviated variable names with descriptive names (team_id not tid, guild_id not gid, workspace_id not wid, chat_id not cid).

This pattern appears across two parallel code blocks (lines 716–737 and 924–945) where abbreviated names are used in match arm bindings. Repo guidelines require descriptive local names: "Don't abbreviate variable names. Use queue not q, message not msg, channel not ch."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/api/bindings.rs` at line 737, In the match arms where you bind Option
tuples (e.g. the snippet showing "Some(tid) => table"), replace abbreviated
local bindings with descriptive names—use team_id instead of tid, guild_id
instead of gid, workspace_id instead of wid, and chat_id instead of cid—so the
match arms and any subsequent uses refer to these full names; update both
parallel blocks that follow the same pattern (the block containing "Some(tid) =>
table" and the corresponding later block) to maintain consistency and
compile-time correctness by renaming the variables wherever they are referenced
in those match arms and their scopes.

src/api/bindings.rs (1)

736-738: Rename abbreviated locals and hoist request normalization outside the loop.

req_team_id is abbreviated and recalculated on each iteration. Prefer explicit naming and compute request normalization once before the for loop.

♻️ Suggested refactor

-    for (i, table) in bindings_array.iter().enumerate() {
+    let normalized_request_team_id = request
+        .original_team_id
+        .as_deref()
+        .map(str::trim)
+        .filter(|value| !value.is_empty());
+    for (i, table) in bindings_array.iter().enumerate() {
@@
-        let req_team_id = request.original_team_id.as_deref().map(str::trim).filter(|s| !s.is_empty());
-        let toml_team_id = table.get("team_id").and_then(|v| v.as_str()).map(str::trim).filter(|s| !s.is_empty());
-        let matches_team = req_team_id == toml_team_id;
+        let normalized_binding_team_id = table
+            .get("team_id")
+            .and_then(|value| value.as_str())
+            .map(str::trim)
+            .filter(|value| !value.is_empty());
+        let matches_team = normalized_request_team_id == normalized_binding_team_id;

As per coding guidelines: "Don't abbreviate variable names. Use queue not q, message not msg, channel not ch."

Also applies to: 938-940

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/api/bindings.rs` around lines 736 - 738, The local name req_team_id is
abbreviated and is being recomputed inside the loop; rename it to
request_team_id and compute the normalized request team once before entering the
for loop by trimming and filtering request.original_team_id.as_deref(), then
inside the loop compute toml_team_id from table.get("team_id") and compare to
request_team_id (use explicit names matches_team or team_matches). Apply the
same renaming/hoisting pattern to the analogous locals at the other site
referenced (lines 938-940) so request normalization is not repeated
per-iteration.

src/messaging/mattermost.rs (2)

91-95: Minor: unnecessary .into() conversion.

Since new() returns anyhow::Result<Self>, and anyhow::anyhow!() already produces an anyhow::Error, the .into() call is a no-op here. Compare with methods returning crate::Result where the conversion is needed.

Suggested cleanup

         if base_url.path() != "/" || base_url.query().is_some() || base_url.fragment().is_some() {
-            return Err(anyhow::anyhow!(
+            anyhow::bail!(
                 "mattermost base_url must be an origin URL without path/query/fragment (got: {})",
                 base_url
-            ).into());
+            );
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 91 - 95, The error return in
Mattermost::new() is performing an unnecessary .into() on an anyhow::Error;
remove the .into() and return the anyhow::anyhow!(...) value directly (i.e., use
Err(anyhow::anyhow!(...)) ) so the return type matches anyhow::Result<Self>
without the redundant conversion.

---

598-618: Use error instead of e for error variables.

Per coding guidelines: "Don't abbreviate variable names."

Suggested fix

-                                        Some(Err(e)) => {
-                                            tracing::error!(adapter = %runtime_key, error = %e, "websocket error");
+                                        Some(Err(error)) => {
+                                            tracing::error!(adapter = %runtime_key, %error, "websocket error");
                                             break;
                                         }
                                         None => break,
                                         _ => {}
                                     }
                                 }
                             }
                         }

                         tracing::info!(adapter = %runtime_key, "websocket disconnected, reconnecting...");
                     }
-                    Err(e) => {
+                    Err(error) => {
                         tracing::error!(
                             adapter = %runtime_key,
-                            error = %e,
+                            %error,
                             delay_ms = retry_delay.as_millis(),
                             "websocket connection failed, retrying"
                         );
                     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/messaging/mattermost.rs` around lines 598 - 618, Rename the short error
binding `e` to `error` in the websocket connection handling match arms to comply
with naming guidelines: update the pattern arms `Some(Err(e))` and `Err(e)` to
`Some(Err(error))` and `Err(error)` respectively, and update all uses in the
tracing macros (e.g., `tracing::error!(adapter = %runtime_key, error = %e, ...
)` and the other tracing call that logs `error = %e`) to reference `%error`;
ensure all occurrences within the surrounding block in mattermost.rs (the
websocket read loop and retry/error logging) are changed consistently so
compilation and log semantics remain the same.